fix(eth): allow eth_call and eth_estimateGas from contract and non-existent senders - #7435
fix(eth): allow eth_call and eth_estimateGas from contract and non-existent senders#7435sudo-shashank wants to merge 17 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughGas estimation and call simulation now support enforced or skipped sender validation. Ethereum RPC paths handle contract and nonexistent senders. State-manager simulation creates ephemeral senders when validation is skipped. Parity tests cover the new behavior. ChangesSender validation flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant eth_estimate_gas
participant GasEstimateGasLimit
participant StateManager
participant call_with_gas
eth_estimate_gas->>GasEstimateGasLimit: select SenderValidation
GasEstimateGasLimit->>StateManager: estimate gas
StateManager->>call_with_gas: simulate message
call_with_gas-->>GasEstimateGasLimit: receipt or SenderValidationFailed
GasEstimateGasLimit-->>eth_estimate_gas: estimate or skipped-validation retry
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/rpc/methods/eth.rs (1)
2109-2124: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winA zero
msg.gas_limitmakes the growth loop run forever.If
msg.gas_limitis0on entry, thenhigh = 0andlow = 0. The conditionhigh < BLOCK_GAS_LIMITholds.can_succeedat limit0fails. Line 2123 then computes0.saturating_mul(2).min(BLOCK_GAS_LIMIT), which is0.highnever grows and the loop never exits. Each iteration performs a full VM execution throughcall_with_gas, so the request thread hangs and consumes CPU without bound.The new
Skippath makes this reachable.eth_estimate_gas_skip_senderderivesgas_limitfromGasEstimateGasLimit::estimate_gas_limit, which returns-1when the receipt is absent (src/rpc/methods/gas.rsLine 286). At Lines 1966-1967 the value becomes((-1i64 as f64) * overestimation) as u64. A negativef64tou64cast saturates to0in Rust, somsg.set_gas_limit(0)runs and0reachesgas_search.Fix the loop so it always makes progress. Also reject the
-1sentinel ineth_estimate_gas_skip_senderbefore you scale it.🐛 Proposed fix
let mut high = msg.gas_limit; let mut low = msg.gas_limit; + // A zero limit would make the doubling below stall at zero. + if high == 0 { + high = 1; + } +Apply this at Lines 1966-1968 so the sentinel never becomes a gas limit:
+ anyhow::ensure!( + gas_limit >= 0, + "gas estimation returned no receipt for a skipped-validation sender" + ); let gas_limit = ((gas_limit as f64 * ctx.mpool.gas_limit_overestimation()) as u64).min(BLOCK_GAS_LIMIT); msg.set_gas_limit(gas_limit);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/rpc/methods/eth.rs` around lines 2109 - 2124, Prevent zero gas limits from stalling gas search and reject the missing-receipt sentinel. In gas_search, ensure the growth loop always advances when high is zero while preserving the BLOCK_GAS_LIMIT cap; in eth_estimate_gas_skip_sender, detect the -1 result from GasEstimateGasLimit::estimate_gas_limit before scaling or calling msg.set_gas_limit, and return the existing appropriate error path instead.
🧹 Nitpick comments (1)
src/rpc/methods/eth.rs (1)
1988-2015: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider accepting the resolved policy as a parameter to avoid a wasted VM execution.
apply_messagealways attemptsSenderValidation::Enforcefirst, then retries withSkip. Callers that already resolved the policy pay for the discarded first execution.
eth_estimate_gas_skip_senderis one such caller. It resolves the policy throughresolve_sender_validationbefore it runs, then its error arm at Line 1956 callsapply_message, which repeats theEnforceattempt and retries. That is two full VM executions on a request already known to needSkip.The PR objective includes benchmarking against Lotus. Adding a
sender_validation: SenderValidationparameter removes the redundant execution on the known-skip path while keeping the detect-and-retry fallback for callers that passEnforce.♻️ Proposed refactor
async fn apply_message( ctx: &Ctx, tipset: Option<Tipset>, msg: Message, + sender_validation: SenderValidation, ) -> Result<ApiInvocResult, Error> { @@ let result = ctx .state_manager .apply_on_state_with_gas( tipset.clone(), msg.clone(), VMFlush::Skip, - SenderValidation::Enforce, + sender_validation, ) .await; - let needs_skip = match &result { + let needs_skip = sender_validation == SenderValidation::Enforce + && match &result { Err(e) => e .downcast_ref::<crate::state_manager::Error>() .is_some_and(|e| matches!(e, crate::state_manager::Error::SenderValidationFailed)), Ok((invoc_res, _)) => invoc_res .msg_rct .as_ref() .is_some_and(|rct| rct.exit_code() == fvm_shared4::error::ExitCode::SYS_SENDER_INVALID), };Then pass
SenderValidation::Skipat Line 1956 andSenderValidation::Enforceat Line 1893.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/rpc/methods/eth.rs` around lines 1988 - 2015, Update apply_message to accept a SenderValidation parameter and use it for the initial apply_on_state_with_gas call, while retaining the existing sender-validation failure detection and retry with Skip when the initial policy is Enforce. Pass SenderValidation::Skip from the resolved-policy error path in eth_estimate_gas_skip_sender and SenderValidation::Enforce from the other apply_message caller.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/rpc/methods/eth.rs`:
- Around line 1927-1934: Update resolve_sender_validation and
estimate_call_with_gas so sender validation uses the same tipset as execution:
either pass the execution tipset from data.mpool.current_tipset() into
resolve_sender_validation, or change execution to use the requested tipset.
Preserve the existing actor-based SenderValidation decisions once both paths
share the same state.
In `@src/tool/subcommands/api_cmd/api_compare_tests.rs`:
- Around line 1651-1669: Update the EthCall and EthEstimateGas cases in the
ApiPaths loop to use strict success assertions instead of
PolicyOnRejected::PassWithIdenticalError, and set msg calldata to a known
non-reverting contract method rather than relying on empty-calldata fallback
behavior. Keep the existing request construction and API-path coverage intact.
---
Outside diff comments:
In `@src/rpc/methods/eth.rs`:
- Around line 2109-2124: Prevent zero gas limits from stalling gas search and
reject the missing-receipt sentinel. In gas_search, ensure the growth loop
always advances when high is zero while preserving the BLOCK_GAS_LIMIT cap; in
eth_estimate_gas_skip_sender, detect the -1 result from
GasEstimateGasLimit::estimate_gas_limit before scaling or calling
msg.set_gas_limit, and return the existing appropriate error path instead.
---
Nitpick comments:
In `@src/rpc/methods/eth.rs`:
- Around line 1988-2015: Update apply_message to accept a SenderValidation
parameter and use it for the initial apply_on_state_with_gas call, while
retaining the existing sender-validation failure detection and retry with Skip
when the initial policy is Enforce. Pass SenderValidation::Skip from the
resolved-policy error path in eth_estimate_gas_skip_sender and
SenderValidation::Enforce from the other apply_message caller.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3ee698fe-9220-4a0c-b643-5281dbb964e6
📒 Files selected for processing (5)
src/rpc/methods/eth.rssrc/rpc/methods/gas.rssrc/state_manager/errors.rssrc/state_manager/message_simulation.rssrc/tool/subcommands/api_cmd/api_compare_tests.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
filecoin-project/lotus(manual)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/tests/api_compare/.env (1)
3-3: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the Lotus baseline consistently across all test environments.
All three files now use the mutable
v1.36.2-calibnettag. Docker tags can be retargeted, which can change parity and benchmark results without a source change. Use one verified immutable digest across all three files. (docs.docker.com)
scripts/tests/api_compare/.env#L3-L3: replace the tag with the pinned digest.scripts/tests/bootstrapper/.env#L2-L2: use the same pinned digest.scripts/tests/snapshot_parity/.env#L1-L1: use the same pinned digest.Verify that the selected digest is the intended Lotus baseline for PR
#13724.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/tests/api_compare/.env` at line 3, Replace the mutable Lotus image tag with the verified immutable digest for the intended PR `#13724` baseline in scripts/tests/api_compare/.env:3-3, scripts/tests/bootstrapper/.env:2-2, and scripts/tests/snapshot_parity/.env:1-1, using exactly the same digest in all three files.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 44-45: Update the changelog entry’s linked reference from pull
request `#7435` to issue `#7394`, preserving the existing description and
formatting.
---
Nitpick comments:
In `@scripts/tests/api_compare/.env`:
- Line 3: Replace the mutable Lotus image tag with the verified immutable digest
for the intended PR `#13724` baseline in scripts/tests/api_compare/.env:3-3,
scripts/tests/bootstrapper/.env:2-2, and scripts/tests/snapshot_parity/.env:1-1,
using exactly the same digest in all three files.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0b2dff37-88ec-40c4-8e44-351eec8ca545
📒 Files selected for processing (6)
CHANGELOG.mdscripts/tests/api_compare/.envscripts/tests/bootstrapper/.envscripts/tests/snapshot_parity/.envsrc/rpc/methods/eth.rssrc/tool/subcommands/api_cmd/api_compare_tests.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
filecoin-project/lotus(manual)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/tool/subcommands/api_cmd/api_compare_tests.rs
- src/rpc/methods/eth.rs
Codecov Report❌ Patch coverage is Additional details and impacted files
... and 12 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
scripts/devnet/.env (1)
10-10: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore dotenv key order.
Move
FOREST_P2P_PORTbeforeFOREST_RPC_PORT.dotenv-linterreportsUnorderedKeyat Line 10.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/devnet/.env` at line 10, Reorder the environment keys in the dotenv configuration so FOREST_P2P_PORT appears before FOREST_RPC_PORT, preserving their existing values.Source: Linters/SAST tools
src/tool/subcommands/api_cmd/api_compare_tests.rs (1)
1651-1782: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd context to fallible test setup.
Import
anyhow::Contextand add.context(...)to the fallible address, calldata, initcode, and request-construction operations in these helpers. Include the affected test case or API method in each message.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tool/subcommands/api_cmd/api_compare_tests.rs` around lines 1651 - 1782, Add anyhow::Context and annotate fallible setup operations in eth_skip_sender_success_tests, eth_skip_sender_insufficient_funds_tests, eth_skip_sender_create_reject_tests, and eth_skip_sender_block_param_tests with contextual errors identifying the relevant test case or API method. Apply context to address, calldata/initcode parsing, and EthCall/EthEstimateGas request construction, including failures propagated through eth_skip_sender_cases.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@scripts/devnet/.env`:
- Line 10: Reorder the environment keys in the dotenv configuration so
FOREST_P2P_PORT appears before FOREST_RPC_PORT, preserving their existing
values.
In `@src/tool/subcommands/api_cmd/api_compare_tests.rs`:
- Around line 1651-1782: Add anyhow::Context and annotate fallible setup
operations in eth_skip_sender_success_tests,
eth_skip_sender_insufficient_funds_tests, eth_skip_sender_create_reject_tests,
and eth_skip_sender_block_param_tests with contextual errors identifying the
relevant test case or API method. Apply context to address, calldata/initcode
parsing, and EthCall/EthEstimateGas request construction, including failures
propagated through eth_skip_sender_cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e5ed9cfb-df83-4111-8be2-8c41afd0b8df
📒 Files selected for processing (7)
CHANGELOG.mdscripts/devnet/.envsrc/rpc/methods/eth.rssrc/rpc/methods/gas.rssrc/state_manager/message_simulation.rssrc/tool/subcommands/api_cmd/api_compare_tests.rssrc/tool/subcommands/api_cmd/test_snapshots.txt
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
filecoin-project/lotus(manual)
🚧 Files skipped from review as they are similar to previous changes (4)
- CHANGELOG.md
- src/rpc/methods/eth.rs
- src/state_manager/message_simulation.rs
- src/rpc/methods/gas.rs
|
@sudo-shashank Did you run your changes against local CC review? I ran it on this PR and it surfaced some potential issues. Are those plausible? |
Yes |
Yes what? Which issues were correctly flagged and fixed, and which did you discard? |
|
Ok, in this case if you're still looking into it, please put the PR to draft. |
3 participants
Summary of changes
Changes introduced in this pull request:
eth_callandeth_estimateGasfrom contract and non-existent addresses via a new skip-sender-validation path, matching Lotus/Geth including tests.Reference issue to close (if applicable)
Closes #7394
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
eth_callandeth_estimateGascompatibility with contract senders and nonexistent accounts.